-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
debug client: support stop ids in from / to inputs #6133
base: dev-2.x
Are you sure you want to change the base?
debug client: support stop ids in from / to inputs #6133
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6133 +/- ##
==========================================
Coverage ? 69.93%
Complexity ? 17727
==========================================
Files ? 1996
Lines ? 75402
Branches ? 7717
==========================================
Hits ? 52730
Misses ? 19995
Partials ? 2677 ☔ View full report in Codecov by Sentry. |
I hope it won't be a big change, so it can be implemented in this pr. 🙂 I think I just need to write a |
@@ -29,6 +29,26 @@ export function SearchBar({ onRoute, tripQueryVariables, setTripQueryVariables, | |||
const [showServerInfo, setShowServerInfo] = useState(false); | |||
const target = useRef(null); | |||
|
|||
const setFromLocation = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfectly fine pattern, but we have done it slightly differently - passing tripQueryVariables and setTripQueryVariables into the input components, and calling them there. I suggest keeping to this for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these functions to the LocationInputField
component.
|
||
const NAME_SEPARATOR = '::'; | ||
|
||
export function parseLocation(value: string): Location | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this function does. In particular, there seems to be something going on with "name" which seems out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These function is the typescript version of the LocationStringParser.java class.
I removed the "name" parsing part.
0233a71
to
fdcd183
Compare
fdcd183
to
a96aba7
Compare
I implemented the I think it's not the frontend's job to decide, which query has to be called to resolve the coordinates, so it would be nice, if the graphql api has a generic place query, that resolves the coordinates for any id. What do you think about it? |
longitude, | ||
latitude, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My knowledge of JS is limited - but it looks like the arguments order is swapped according to the interface definition above. As a practice to avoid errors I like to always do lat, long
never long, lat
(doc, params, args, execution order and so on).
There are still unresolved issues in this PR imho. Maybe need to discuss in developer meeting? Although I'm unable to join until week after next week. |
Summary
Added support for location ids in from/to fields in debug client.
Implementation Details:
locationConverter
: converts strings to Location objects and back based onLocationStringParser
class.LocationInputField
: a value state introduced for the input field. The newuseEffect
hook updates this state when location parameter changes. On theonBlur
event, this component fires the newonLocationChange
event if a valid location can be parsed from the input.SearchBar
: updatestripQueryVariables
based on the changed location.